Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARC-57: Application Action Routing #264

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

joe-p
Copy link
Contributor

@joe-p joe-p commented Jan 2, 2024

This app provides standardized TEAL that a developer can use to allow tools such as explorers or wallets easily determine if an app can be updated or deleted (among other actions)

err
```

For every action (combination of `ApplicationID === 0` and `OnCompletion`) there is a label that corresponds to that action. If that action is not implemented in the contract, its label should be replaced by a label that simply contains `err`. In the above TEAL, this is the `NOT_IMPLEMENTED` label.
Copy link
Contributor

@jannotti jannotti Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An offchain parser has to check the destination of the label, that it contains err only? This is going to lead to extreme fragility in that we now have to worry about badly written parsers of TEAL code. If we change the disassembly output slightly (for example, add comments to a line, as we sometimes do), will they break? If not, they must be implementing a fully correct TEAL parser?

Since you're asking them to skip constant blocks, they have to handle all possible ways to write constants. There's base32 and base64 strings, for example.

This entry point is certainly one way to do things, and I know TealScript does. But nothing else does, and it doesn't seem best for immutable contracts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An offchain parser has to check the destination of the label, that it contains err only? This is going to lead to extreme fragility in that we now have to worry about badly written parsers of TEAL code. If we change the disassembly output slightly (for example, add comments to a line, as we sometimes do), will they break? If not, they must be implementing a fully correct TEAL parser?

As long as nothing is added before the opcodes then parses shouldn't have a problem if they check to ensure only the beginning matches. I can update the ARC to be more clear about this.

Since you're asking them to skip constant blocks, they have to handle all possible ways to write constants. There's base32 and base64 strings, for example.

I'm not sure I follow. If parsers ignore bytecblock and intcblock lines there aren't other types of constant blocks, are there? I know there are pseudo-ops for different encodings, but aren't those ultimately compiled to either pushbytes or put in the bytecblock?

This entry point is certainly one way to do things, and I know TealScript does. But nothing else does, and it doesn't seem best for immutable contracts.

A side benefit of this approach is that you can check what other "actions" are supported, but I don't think that's very important.

The alternative, as you suggested on discord would be asserts in the beginning. Parsers would check for txn OnComplete; pushint DeleteApplication; !=; assert or txn OnComplete; dup; pushint DeleteApplication; !=; assert; pushint UpdateApplication; != assert (I think checking for update is just as important as being able to check for delete). The main downside of this approach is extra ops/bytes in the program, but it's not a lot.

@FrankSzendzielarz
Copy link

What's the motivation behind this? The stated motivation doesn't explain what problem is being solved or goal is being achieved.

@joe-p
Copy link
Contributor Author

joe-p commented Jan 5, 2024

Just updated the motivation and rationale sections

Co-authored-by: nullun <[email protected]>
@SudoWeezy SudoWeezy changed the title ARC-XXXX: Application Action Routing ARC-57: Application Action Routing Jan 8, 2024
This ARC provides a standard way to handle an application call depending on the combination of the `OnCompletion` and `ApplicationID`.

## Motivation
App mutability is an important factor in a rational actors decision making when it comes to interacting with an app and sending funds to a contract account. If a contract is mutable, it is possible that funds could be stolen or lost after the initial program is changed or deleted. Currently, there is no easy way for an end-user to determine the mutability of a contract. It is possible to use static analysis on the decompiled TEAL, but this is not easily accessible and unrealistic for wallets and explorers to implement for every program.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

App mutability is an important factor in a rational actors decision making when it comes to interacting with an app and sending funds to a contract account. If a contract is mutable, it is possible that funds could be stolen or lost after the initial program is changed or deleted.

I'm not really sure I see the point of this ARC as even an immutable contract could steal your funds if you don't understand all the code. It might just provide a false sense of security.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no way to have 100% security guarantees. A bug in algod could result in loss of funds, but that doesn't mean we should discount all other security considerations.

if you don't understand all the code

The goal of this ARC is to make it easier to understand critical parts of the code.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but what about existing immutable contracts that don't have this code pattern and can't be updated to use it. They miss out on this 'trusted' badge.

At first glance it also doesn't look like this pattern will work with the existing beaker template variables for TMPL_UPDATABLE and TMPL_DELETABLE. Obviously there are alternative solutions to this - perhaps this ARC should describe them?

I think ultimately this would be better resolved at the AVM level with a new opcode/pragma comment at the top of the program.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Support the idea of dealing with this at avm level. I have seen chat recently about legal issues of mutable contracts, so the requirement seems to be one of some consequence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants